Skip to content

feat: Add FFI_QueryPlanner to support foreign query planners across shared-library boundaries#22151

Draft
milenkovicm wants to merge 1 commit into
apache:mainfrom
milenkovicm:poc_ffi_query_planner_RELOADED
Draft

feat: Add FFI_QueryPlanner to support foreign query planners across shared-library boundaries#22151
milenkovicm wants to merge 1 commit into
apache:mainfrom
milenkovicm:poc_ffi_query_planner_RELOADED

Conversation

@milenkovicm
Copy link
Copy Markdown
Contributor

@milenkovicm milenkovicm commented May 13, 2026 •
edited
Loading

Which issue does this PR close?

This is follow up on #20249 which was co-authored with @timsaucer

Closes #. (pending)

Rationale for this change

DataFusion's FFI layer allows plugins and foreign libraries to extend query processing across shared-library boundaries. However, there was no way for a foreign library to participate in the physical planning pipeline — QueryPlanner required a concrete &SessionState, which cannot cross an FFI boundary.

PR rallies on dyn Session which limits use of this functionality, as PhysicalPlanner, which in most cases would be called, expects SessionState and in FFI environments it would be unsafe to cast dyn Session to SessionState. Proposed change would unblock Ballista integration with DF python, but apart from that, I'm not sure how useful it would be until we align other important interfaces to use dyn Session.

What changes are included in this PR?

  • Adds FFI_QueryPlanner — an FFI-safe repr-C struct that wraps a foreign QueryPlanner implementation, serializing the LogicalPlan via datafusion-proto across the boundary and deserializing the resulting ExecutionPlan.
  • Adds QueryPlannerWeak trait — a variant of QueryPlanner that accepts &dyn Session instead of &SessionState, making it implementable by foreign code. It make more sense to create new interface rather than changing QueryPlanner. The change would introduce big backward incompatibility, and limiting use cases this functionality could be used, Also, we can argue that having SessionState as parameter makes sense.
  • Extends QueryPlanner with an Any bound to allow downcasting (needed by DynamicForeignQueryPlanner).
  • Adds integration tests covering both direct use of FFI_QueryPlanner via QueryPlannerWeak and the deferred-injection pattern via DynamicForeignQueryPlanner.

The DynamicForeignQueryPlanner helper (in the integration tests) shows how to break the construction-time dependency cycle: SessionContext needs a QueryPlanner at build time, but the FFI planner needs the codec which needs the context. The placeholder is registered at build time and swapped for the real FFI planner once both sides exist.

Are these changes tested?

Yes. Integration tests are added in datafusion/ffi/tests/ffi_planner.rs:

  • test_ffi_query_planner — verifies that a foreign planner loaded from a shared library can produce a valid ExecutionPlan from a LogicalPlan.
  • test_ffi_dynamic_query_planner — verifies the deferred-injection pattern where the FFI planner is installed into an already-constructed SessionContext via DynamicForeignQueryPlanner.

Are there any user-facing changes?

Yes, one change to a public trait:

  • QueryPlanner now requires Any as a supertrait (enables downcasting). This is not required for this implementation but it might be needed for implementors

Open Questions

  • I believe this is not generic enough; mix of SessionState and dyn Session usage across internal interfaces & (physical) planners makes this proposal not really useful until we move all relevant traits to dyn Session; question is do we move this to datafusion-python to enable ballista use cases?
  • there is chicken-egg problem at the moment, SessionContext is needed to create a QueryPlanner but QueryPlanner is required to create a SessionContext (I hope I did not get that horrible wrong)
  • Name of QueryPlannerWeak is debatable, I cant come up with better name

@milenkovicm milenkovicm requested a review from timsaucer May 13, 2026 17:01
@github-actions github-actions Bot added core Core DataFusion crate ffi Changes to the ffi crate labels May 13, 2026
@milenkovicm milenkovicm changed the title feat(ffi): Add FFI_QueryPlanner to support foreign query planners across shared-library boundaries feat: Add FFI_QueryPlanner to support foreign query planners across shared-library boundaries May 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026 •
edited
Loading

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details 
     Cloning apache/main
    Building datafusion v53.1.0 (current)
       Built [ 102.481s] (current)
     Parsing datafusion v53.1.0 (current)
      Parsed [   0.036s] (current)
    Building datafusion v53.1.0 (baseline)
       Built [ 102.120s] (baseline)
     Parsing datafusion v53.1.0 (baseline)
      Parsed [   0.036s] (baseline)
    Checking datafusion v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.635s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip

--- failure trait_added_supertrait: non-sealed trait added new supertraits ---

Description:
A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/trait_added_supertrait.ron

Failed in:
  trait datafusion::execution::context::QueryPlanner gained Any in file /home/runner/work/datafusion/datafusion/datafusion/core/src/execution/context/mod.rs:2077

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [ 206.888s] datafusion
    Building datafusion-ffi v53.1.0 (current)
       Built [  58.568s] (current)
     Parsing datafusion-ffi v53.1.0 (current)
      Parsed [   0.059s] (current)
    Building datafusion-ffi v53.1.0 (baseline)
       Built [  59.116s] (baseline)
     Parsing datafusion-ffi v53.1.0 (baseline)
      Parsed [   0.058s] (baseline)
    Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.229s] 222 checks: 220 pass, 1 fail, 1 warn, 30 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field ForeignLibraryModule.create_query_planner in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:105

--- warning repr_c_plain_struct_fields_reordered: struct fields reordered in repr(C) struct ---

Description:
A public repr(C) struct had its fields reordered. This can change the struct's memory layout, possibly breaking FFI use cases that depend on field position and order.
        ref: https://doc.rust-lang.org/reference/type-layout.html#reprc-structs
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/repr_c_plain_struct_fields_reordered.ron

Failed in:
  ForeignLibraryModule.version moved from position 15 to 16, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:108

     Summary semver requires new major version: 1 major and 0 minor checks failed
     Warning produced 1 major and 0 minor level warnings
    Finished [ 119.378s] datafusion-ffi

@github-actions github-actions Bot added the auto detected api change Auto detected API change label May 13, 2026
@milenkovicm milenkovicm force-pushed the poc_ffi_query_planner_RELOADED branch from ee61d0f to 8e23f2c Compare May 13, 2026 21:29
@milenkovicm @timsaucer
Initial implementation of FFI_QueryPlanner
d49d1bc 
Co-authored-by: Tim Saucer <timsaucer@gmail.com>
@milenkovicm milenkovicm force-pushed the poc_ffi_query_planner_RELOADED branch from 8e23f2c to d49d1bc Compare May 14, 2026 13:13
@milenkovicm milenkovicm marked this pull request as draft May 15, 2026 20:53
@milenkovicm
Copy link
Copy Markdown
Contributor Author

milenkovicm commented May 15, 2026

At the moment I believe this is more like a hack to enable ballista working with datafusion python, than a systematic solution. Perhaps it should be moved to datafusion python to support that single use case until datafusion makes decision do we use SessionState or dyn Session across key interfaces.

Thus, I'd like to suggest moving this functionality to datafusion python. Would appreciate your opinion @timsaucer, @alamb & @andygrove

@milenkovicm milenkovicm requested review from alamb and kosiew May 16, 2026 06:53
@milenkovicm milenkovicm marked this pull request as ready for review May 16, 2026 07:33
@milenkovicm milenkovicm mentioned this pull request May 16, 2026
Open
@milenkovicm milenkovicm marked this pull request as draft May 16, 2026 07:38
@timsaucer
Copy link
Copy Markdown
Member

timsaucer commented May 18, 2026

As I was digging in, I found what I mentioned before wanting to change around. The root problem that I think needs to be reworked is making it so that we can pass &dyn Session instead of &SessionState in create_physical_plan. We'd need to move QueryPlanner up out of core so that we don't re-pollute the FFI crate. This means we need to add optimize_physical_plan to Session and a few other mechanical moving things around.

If we did this then I think the entire approach in this crate would be a lot cleaner.

I'm discovering right now that we also have an issue with the new scalar subquery data structure, so I've got my agent working away at that right now.

@milenkovicm
Copy link
Copy Markdown
Contributor Author

milenkovicm commented May 18, 2026

Yes, that’s the one I had trouble with last time I tried this.

@timsaucer timsaucer mentioned this pull request May 18, 2026
Draft
@alamb
Copy link
Copy Markdown
Contributor

alamb commented May 18, 2026

If we did this then I think the entire approach in this crate would be a lot cleaner.

Sounds like a reasonable approach to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timsaucer timsaucer Awaiting requested review from timsaucer

@alamb alamb Awaiting requested review from alamb

@kosiew kosiew Awaiting requested review from kosiew

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

auto detected api change Auto detected API change core Core DataFusion crate ffi Changes to the ffi crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@milenkovicm @timsaucer @alamb